Skip to content

Continuous Benchmarking [Addendum] #1034

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Continuous Benchmarking [Addendum] #1034

wants to merge 1 commit into from

Conversation

@Malmahrouqi3
Copy link
Member

@Malmahrouqi3 Malmahrouqi3 commented Nov 10, 2025 •
edited by qodo-code-review bot
Loading

User description

Description

Following on #1033,
This PR should be merged after pushing a finalized data.js (containing historic benchmark runs) to MFlowCode.github.io under documentation dir. Then #1033 will display whether bench_diff works as intended or not with the Self-Hosted Benchmark Tests (3 GT Phoenix, 1 ORNL Frontier).

PR Type

Enhancement

Description

  • Preserve existing benchmark data during documentation deployment

  • Prevent overwriting historical data.js in documentation repository

  • Ensure continuous benchmarking data persists across doc updates

Diagram Walkthrough

flowchart LR
  A["Docs workflow"] --> B["Clone documentation repo"]
  B --> C["Clear old files"]
  C --> D["Copy new docs"]
  D --> E["Stage all changes"]
  E --> F["Restore data.js"]
  F --> G["Commit and push"]
Loading

File Walkthrough
Relevant files
Enhancement
docs.yml
Restore benchmark data file during doc deployment               

.github/workflows/docs.yml

  • Added git restore documentation/data.js command after staging changes
  • Prevents historical benchmark data from being overwritten during doc
    deployment
  • Ensures data.js file is preserved from the remote repository
 +1/-0     

Copilot AI review requested due to automatic review settings November 10, 2025 19:56
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Nov 10, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

The restore command assumes that a git-tracked 'documentation/data.js' exists in the target repo after removing everything with 'rm -rf ../www/*'. If the file isn't present or path differs, the restore will fail or silently leave it missing; consider restoring before wiping, or excluding the file from deletion.

 
if [ "$?" -ne "0" ]; then exit 0; fi
set -e
git config --global user.name  'MFC Action'
git config --global user.email '<>'
git clone "${{ secrets.DOC_PUSH_URL }}" ../www
rm -rf ../www/*
mv build/install/docs/mfc/* ../www/
git -C ../www add -A
git -C ../www restore documentation/data.js
git -C ../www commit -m "Docs @ ${GITHUB_SHA::7}" || true
git -C ../www push
Fragile Path

Hard-coding 'documentation/data.js' may break if the documentation structure changes; consider parameterizing the path or using a .gitignore/.gitattributes approach to persist history data.

 
if [ "$?" -ne "0" ]; then exit 0; fi
set -e
git config --global user.name  'MFC Action'
git config --global user.email '<>'
git clone "${{ secrets.DOC_PUSH_URL }}" ../www
rm -rf ../www/*
mv build/install/docs/mfc/* ../www/
git -C ../www add -A
git -C ../www restore documentation/data.js
git -C ../www commit -m "Docs @ ${GITHUB_SHA::7}" || true
git -C ../www push

qodo-code-review[bot]
qodo-code-review bot reviewed Nov 10, 2025
View reviewed changes
.github/workflows/docs.yml
Comment on lines 71 to +72
git -C ../www add -A
git -C ../www restore documentation/data.js
Copy link
Contributor

@qodo-code-review qodo-code-review bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Move the git restore command before git add -A to ensure changes to documentation/data.js are correctly discarded before being staged for commit. [possible issue, importance: 9]

Suggested change
git -C ../www add -A
git -C ../www restore documentation/data.js
git -C ../www restore documentation/data.js
git -C ../www add -A

Copilot AI reviewed Nov 10, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a safeguard to preserve historic benchmark data (documentation/data.js) during automated documentation deployments. It's a follow-up to #1033 that implements continuous benchmarking functionality.

Key Changes:

  • Adds a git restore command to prevent documentation/data.js from being overwritten during documentation builds

.github/workflows/docs.yml
rm -rf ../www/*
mv build/install/docs/mfc/* ../www/
git -C ../www add -A
git -C ../www restore documentation/data.js
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The git restore command will fail if documentation/data.js doesn't exist in the git history of the ../www repository (e.g., during initial setup or if the file hasn't been pushed yet). Since set -e is enabled on line 65, this will cause the workflow to fail.

Consider adding error handling to gracefully handle the case where the file doesn't exist:

git -C ../www restore documentation/data.js || true

Alternatively, check if the file exists in the git history before attempting to restore it:

git -C ../www ls-files --error-unmatch documentation/data.js &>/dev/null && git -C ../www restore documentation/data.js || true
Suggested change
git -C ../www restore documentation/data.js
git -C ../www restore documentation/data.js || true

Copilot uses AI. Check for mistakes.
.github/workflows/docs.yml
git clone "${{ secrets.DOC_PUSH_URL }}" ../www
rm -rf ../www/*
mv build/install/docs/mfc/* ../www/
git -C ../www add -A
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider adding a comment to explain why documentation/data.js is being restored. Based on the PR description, this file contains historic benchmark data that should be preserved across documentation deployments. A brief comment would improve maintainability and help future developers understand this special handling.

Example:

# Preserve historic benchmark data from continuous benchmarking (see #1033)
git -C ../www restore documentation/data.js || true
Suggested change
git -C ../www add -A
git -C ../www add -A
# Preserve historic benchmark data from continuous benchmarking (see #1033)

Copilot uses AI. Check for mistakes.
@sbryngelson sbryngelson marked this pull request as draft November 29, 2025 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@qodo-code-review qodo-code-review[bot] qodo-code-review[bot] left review comments

Copilot code review Copilot Copilot left review comments

@sbryngelson sbryngelson Awaiting requested review from sbryngelson sbryngelson is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Review effort 1/5

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Malmahrouqi3